Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automate release notes generation #6187

Merged
merged 26 commits into from
Sep 16, 2021
Merged

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented May 8, 2021

Summary

See #5520

This PR adds a custom GHA that will create (or update) a release draft for the specified branch and milestone. A new GHA workflow has also been added that allows one to manually trigger the action.

The release name and tag are derived from amp.php, and the release notes are generated from the given milestone.

The action also provides an output called asset_upload_url, that would allow other GHA jobs to upload release assets to the created release (which we can use to upload a production build of the plugin, for example).

Screenshots

  • What it would look like when triggering the workflow:

    image
  • An example of what the action logs:

    image
  • An example of a draft release:

    image

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@pierlon pierlon self-assigned this May 8, 2021
@pierlon pierlon changed the title Automate release management tasks Automate release notes generation May 8, 2021
@pierlon pierlon added the Infrastructure Changes impacting testing infrastructure or build tooling label May 8, 2021
@pierlon pierlon force-pushed the enhancement/5520-release-automation branch from 667ca54 to 5d2e6cf Compare May 8, 2021 11:37
@pierlon
Copy link
Contributor Author

pierlon commented May 8, 2021

The "Release Drafter / Update release draft" workflow is failing because the necessary config is not yet on the develop branch. One of the developers of the Action stated it's a known limitation (release-drafter/release-drafter#398 (comment)):

This is a limitation inside probot this is properly intended behavior so that somebody cannot introduce something from a pull requests or from a branch.

@pierlon pierlon marked this pull request as ready for review June 29, 2021 15:02
@github-actions
Copy link
Contributor

github-actions bot commented Jun 29, 2021

Plugin builds for 3c87818 are ready 🛎️!

pierlon and others added 3 commits June 29, 2021 11:19
…nt/5520-release-automation

* 'develop' of github.com:ampproject/amp-wp:
  Fix test_get_mu_plugins_data for WP 5.8 (#6434)
  Rename prop
  Remove the QA Tester plugin
  Bump eslint from 7.28.0 to 7.29.0
  See if removing the expression syntax allows for the step conditionals to  be evaluated correctly
  Update Gutenberg package dependencies
  Add test for amp-render[template]
  Update amphtml spec to 2106240350000
  Bump sirbrillig/phpcs-variable-analysis in /qa-tester
  Bump optimize-css-assets-webpack-plugin from 6.0.0 to 6.0.1
  Bump eslint-plugin-jsdoc from 34.6.1 to 35.4.0
@pierlon
Copy link
Contributor Author

pierlon commented Jun 30, 2021

I've added a npm script to allow for the release changelog to be generated locally as well. The only caveat is that you would need a GitHub PAT (with the permissions public_repo, read:org, read:user, repo:status, user:email) in order access GitHub's GraphQL API.

The script accepts the milestone to generate the changelog for and gets the token from the GITHUB_TOKEN environment variable. The current syntax to run the script is (assuming GITHUB_TOKEN environment variable has already been set):

npm run generate-changelog -- <milestone name>


const milestone = process.argv[ 2 ];

const token = process.env.GITHUB_TOKEN;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If token is empty, should this raise an error to inform the user to add it to their environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that makes sense. I've added some logic to properly parse the args and validate them in 312f0d3.

description: 'The URL for uploading assets to the release'
runs:
using: 'node12'
main: 'dist/index.js'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this file come from? Forgive my ignorance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, nevermind. It comes from npm run build for this sub-package.

Comment on lines 21 to 23
if (matches && matches[1]) {
[, tagName] = matches;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (matches && matches[1]) {
[, tagName] = matches;
}
if (!matches || !matches[1]) {
throw new Error( 'Unable to parse Version from plugin bootstrap PHP file.' );
}
tagName = matches[1];

if (matches && matches[1]) {
[, tagName] = matches;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worthwhile to do some checking of the milestone to make sure it matches:

Suggested change
if ( ! tagName.startsWith( tagName.substr(1) ) ) {
throw new Error( "Milestone mismatch with PHP plugin bootsrap version." );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 752872d.

target_commitish: commitish
});
core.info(`Created ${tagName} release`);
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should check to see if release is a draft, and if not, throw an error. We don't want to accidentally update a previous release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 5b6e433.

@westonruter
Copy link
Member

Looking forward to seeing this in action!

@lgtm-com
Copy link

lgtm-com bot commented Sep 15, 2021

This pull request introduces 1 alert when merging 1a89432 into 6822183 - view on LGTM.com

new alerts:

  • 1 for Unsafe HTML constructed from library input

@westonruter
Copy link
Member

This pull request introduces 1 alert when merging 1a89432 into 6822183 - view on LGTM.com

I guess this reflects an update to LGTM's audits, rather than a code change here. This PR didn't touch amp-customize-controls.js. I've marked the test failure as a false-positive since the contents are escaped using wp_kses_post().

So the package conflicts just have to be fixed… again. 😬

@lgtm-com
Copy link

lgtm-com bot commented Sep 16, 2021

This pull request introduces 1 alert when merging 0c958d7 into 7dd4c1a - view on LGTM.com

new alerts:

  • 1 for Unsafe HTML constructed from library input

@pierlon
Copy link
Contributor Author

pierlon commented Sep 16, 2021

Hmm seems the error is back again. Wondering if it can be hidden permanently.

@lgtm-com
Copy link

lgtm-com bot commented Sep 16, 2021

This pull request introduces 1 alert when merging 718b950 into 7dd4c1a - view on LGTM.com

new alerts:

  • 1 for Unsafe HTML constructed from library input

@lgtm-com
Copy link

lgtm-com bot commented Sep 16, 2021

This pull request introduces 1 alert when merging 3c87818 into 7dd4c1a - view on LGTM.com

new alerts:

  • 1 for Unsafe HTML constructed from library input

@pierlon
Copy link
Contributor Author

pierlon commented Sep 16, 2021

OK so according to the LGTM docs, one should be able to disable individual alerts by adding "suppression comments" to the source code, which I've done in 3c87818.

One thing to note from that same doc:

If you've enabled automated code review of pull requests for your projects, it's important to note that alert suppression changes made in a pull request are only applied when that pull request is merged. So, if you suppress an alert in a pull request, LGTM will continue to report the related alert for as long as the pull request is open.

So it's expected to still get alerts in the PR if the alert was disabled.

@pierlon pierlon merged commit 26c3283 into develop Sep 16, 2021
@pierlon pierlon deleted the enhancement/5520-release-automation branch September 16, 2021 01:22
@westonruter westonruter modified the milestones: v2.2, v2.3 Dec 7, 2021
@westonruter westonruter modified the milestones: v2.3, v2.4 Dec 23, 2021
@westonruter westonruter modified the milestones: v2.4, v2.3 Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Changes impacting testing infrastructure or build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants